Skip to content

fix(desktop): open supported Skill directories - #5273

Merged
liuxiaocs7 merged 11 commits into
apache:mainfrom
liuxiaocs7:fix/5272-desktop-skill-locations
Sep 19, 2026
Merged

fix(desktop): open supported Skill directories#5273
liuxiaocs7 merged 11 commits into
apache:mainfrom
liuxiaocs7:fix/5272-desktop-skill-locations

Conversation

@liuxiaocs7

@liuxiaocs7 liuxiaocs7 commented Sep 13, 2026

Copy link
Copy Markdown
Member

Summary

Desktop exposed one ambiguous Open Skills folder action even though the Runtime catalog discovers five standard directories. When the legacy workspace directory was absent, the action failed with “directory does not exist” instead of opening the location that owns a Skill.

Replace that action with a Skill locations… submenu covering the two project paths, the legacy workspace path, and the two user paths. Existing directories open directly; missing allowlisted directories can be created and opened. The renderer sends only a location ID, and the main process resolves it with containment checks. Remote Runtime Hosts continue to hide local path actions, while individual Skill actions retain their real source path.

Fixes #5272

Verification

  • npm run typecheck
  • npm run lint
  • npm run format:check
  • npx knip --workspace apps/desktop
  • npx knip --workspace packages/ui
  • npm --workspace @maka/desktop run typecheck:stories
  • npm --workspace @maka/desktop run build-storybook
  • npm --workspace @maka/desktop run smoke:storybook — 359 stories / 388 theme renders passed
  • npm --workspace @maka/ui run test:dist — 453 passed
  • Focused Desktop tests — 22 passed

The root test run also exercised all workspaces. Unrelated existing failures remain in the local environment: Eval's Python tests run under Python 3.9 and cannot parse X | None, and one storage cross-process test treats Node's SQLite experimental warning as an error.

AI use

  • No generative tool made a substantive contribution
  • Generative tooling made a substantive contribution

Tool(s) and scope: Codex implemented and reviewed the Desktop/UI change, tests, localization, and path-containment handling.

Checklist

  • Tests cover the change and fail without it
  • Lint, format, typecheck and the affected suites pass locally

Does this PR entail a change in behavior?

  • Yes — described under Summary above
  • No

@github-actions github-actions Bot added the effort/L Under 1000 readable lines label Sep 13, 2026
@liuxiaocs7
fix(desktop): bind Skill locations to their project context
1ee5c2f 
Reject stale project directory actions, preserve unreadable location diagnostics, and keep Storybook location counts consistent with inventory.

Regenerate renderer architecture and Astryx inventories to satisfy CI gates.

Generated-by: Codex
@liuxiaocs7
test(desktop): register POSIX Skill permissions coverage
465f023 
Record the platform-specific skip in the generated Windows test inventory.

Generated-by: Codex
@github-actions github-actions Bot added effort/XL Under 2500 readable lines and removed effort/L Under 1000 readable lines labels Sep 14, 2026
@liuxiaocs7
Merge origin/main into fix/5272-desktop-skill-locations
e25f4da 
Regenerate renderer architecture and Windows test inventories from the merged source, preserving Skill location context and permissions coverage.

Validated with a clean build, typecheck, Biome, Knip, strict inventories, 2709 Desktop tests, 499 UI tests, and Storybook smoke (379 stories / 411 theme renders).

Generated-by: Codex
@liuxiaocs7
Merge latest origin/main test fixes
ba1853a 
Include upstream Runtime Host PTY integrity and two-client CAS deflakes from 19971f3. Both changed test files pass locally.

Generated-by: Codex
@liuxiaocs7
fix(desktop): isolate Skill location scopes from missing projects
631bb70 
Resolve local Skill locations independently of the governance catalog and bind opens to Host-scoped canonical roots. Keep workspace and user locations available when the selected Project disappears, without recreating its path.

Generated-by: Codex
@liuxiaocs7
Merge origin/main and refresh renderer architecture ledger
6905537 
Preserve the Skill location scope fix and upstream Composer Session-reference support. Regenerate the shared AppShell token count from the merged source and re-pin the compatible cwd-limit declaration to upstream epoch 163.

Generated-by: Codex
Astro-Han
Astro-Han approved these changes Sep 18, 2026
View reviewed changes

@Astro-Han Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The trust boundary is correctly drawn. The renderer can only echo an opaque ref plus {contextId, createIfMissing}; contextId is sha256(registrationSeed, scope, canonicalRoot, projectId) minted main-side, and open re-derives and compares it (stale_context on any mismatch — project switch, root remount, host re-registration each invalidate). resolveSkillLocation/ensureContainedDirectory re-inspect with lstat + realpath, refuse symlinked leaf directories, and only a canonical contained path ever reaches shell.openPath. No arbitrary-path or traversal route exists from the renderer. The remote-host gate is layered consistently (allowLocalPaths in main and clientPathsAccessible in the renderer), listing derives from the same resolveSkillDiscoveryPaths enumeration the runtime uses, and the open allowlist is a deliberately narrower, fail-closed subset. Controller tests drive the real port shape; main tests pin contextId scoping, project-switch invalidation, canonical-path opening after a root remount, and remote interception. diag:open-skills is retired cleanly with nav:skills covering navigation, and copy is complete across all three locales.

All findings are P3.

  • The 'skills' key on the legacy app:openPath channel is now dead (open-path-guard.ts still maps it to {workspaceRoot}/skills, openPathLabels.skills/paths.skills/OpenPathKey remain in the renderer) — its only caller was removed here, and the dead path is looser than the new channel (leaf symlinks pass realpath-check where the new flow refuses them). Prefer removing it in this PR or noting why it stays.
  • "Supported location" is encoded in three places that must agree by hand: resolveSkillDiscoveryPaths entries, skillLocationScope's hardcoded ref list, and the SkillLocationRef union in module-panel-types.ts (with an unchecked as cast at skillLocationRef). A future sixth location would list with an undefined label and refuse open as unknown_location — fail-closed but silently drifting; one ref table or resolving scope from the found entry would collapse it to one declaration.
  • locations[].path shows the non-canonical entry.dir while open uses the canonical realpath — display-only divergence under symlinked roots.
  • Small test gaps at the IPC layer: open_failed (openPath error), create_failed (mkdir EPERM), and leaf-symlink blocked_path are pinned only at resolveSkillLocation, not through the handler.
  • Renderer nits: stale_context/missing failures toast but don't auto-refreshSkillLocations; refreshSkillLocations fires one wasted IPC for remote hosts; location menu items invoke runSkillAction synchronously where sibling native-window actions defer via runPageActionAfterMenuClose; openLocationFailures.stale_context copy is project-scoped for a reason that can fire on other scopes; skills-panel-locations.test.tsx doesn't pin the createIfMissing: false arm.
  • Unrelated churn to trim or annotate: the external-session-cwd-limit-authority.json epoch re-pin (merge bookkeeping — its target file is not in this diff) and whitespace-only deletions in session-inspector/service.ts and conversation-copy.ts.

@liuxiaocs7
fix(desktop): address Skill location review feedback
cdca2eb 
Retire the legacy Skills open-path key, share standard location identities across Runtime and Desktop, and display canonical opening paths. Refresh invalid location snapshots without retrying mutations, skip remote location reads, defer menu actions, and extend IPC/controller/UI regression coverage.

Generated-by: Codex
@liuxiaocs7
fix(desktop): refresh Skill locations when local paths become available
00b30ea 
Preserve the deferred startup refresh while loading locations when asynchronous Project capabilities arrive. Clear stale location snapshots on capability revocation and refresh on restoration without reloading the other Skill projections. Cover capability readiness before and after the startup frame through the Module Hub controller lifecycle.

Generated-by: Codex
@liuxiaocs7

liuxiaocs7 commented Sep 18, 2026 •
edited
Loading

Copy link
Copy Markdown
Member Author

Codex automated follow-up for @Astro-Han's review, implemented in cdca2eb, 00b30ea, and 443605d:

  1. Legacy route removed. Removed the skills key from app:openPath, both bridge contracts, renderer types, and all three locale catalogs. A regression test confirms even an existing compatibility directory now returns unknown-key through that retired key.
  2. One location declaration. @maka/core/skill-locations is the shared, filesystem-free table for Runtime discovery and Desktop's opening allowlist. UI refs and scope/source types derive from it; the unchecked casts and hardcoded scope switch are gone. Discovery precedence remains unchanged.
  3. Canonical displayed paths. Available and creatable missing locations now show the canonical path used for opening/creation, including symlinked roots. IPC tests compare the refreshed displayed paths with the paths passed to the shell.
  4. IPC failure coverage. Added handler-level tests for shell open_failed, permission-denied create_failed, and blocked_path for leaf symlinks pointing both inside and outside the root. The POSIX permissions test is documented in the Windows inventory.
  5. Renderer follow-ups. stale_context and missing refresh the snapshot without retrying the original open/create. Remote refreshes do not issue location IPC; asynchronous local-path capability readiness/restoration now automatically loads locations, while revocation invalidates pending snapshots. Startup remains animation-frame deferred, including under StrictMode, with tests for capabilities ready at mount, before the frame, and after the frame. Host/generation/surface fences remain covered. Location menu actions use runPageActionAfterMenuClose, the three locale messages are scope-neutral, and the component test covers both createIfMissing arms plus close-before-action ordering.
  6. Merge bookkeeping annotated, not reverted. Kept epoch 163 because upstream moved the protocol epoch and the compatibility declaration must match it (parseDeclaration explicitly rejects a stale epoch). This was already documented in merge commit 6905537. The two whitespace-only deletions also remain: restoring their blank EOF lines reproduced git diff --check failures (new blank line at EOF), which the mandatory pre-commit hook runs. Neither changes behavior; this follow-up adds no changes to either file.

Verification on the updated tree:

  • Focused regression suite: 90/90.
  • Complete Desktop suite after the final StrictMode fix: 2,735/2,735; UI: 507/507.
  • Root build/typecheck/lint/format, Desktop/UI knip, strict-base renderer architecture, ASF headers, and generated inventories passed.
  • Rebuilt Storybook; focused extensions-skills-locations, extensions-skills-narrow, and host-extensions-skills browser smoke passed, including existing/missing menu actions.
  • Ran every workspace's suite. Six Host/Storage tests failed in the full run; five passed when rerun serially. The remaining production Host publishes and retires an implementation child patch still times out with Hosted real-model Turn did not become terminal, matching the earlier local validation before this follow-up. Eval's 87 Python tests passed with Python 3.12. The suite used a scoped suppression of Node's experimental SQLite warning; no test code or timeout was changed to hide failures.

The earlier nonblocking UI-attachment gap remains: before/after evidence and the current menu screenshots have been captured locally, but are not attached to this PR yet. No Electron/Windows end-to-end result is claimed.

@liuxiaocs7
fix(desktop): preserve deferred Skill reads in StrictMode
443605d 
Track whether locations were requested independently of the generation fence, which also advances during StrictMode cleanup. Cover at-mount and asynchronous capability readiness under the real StrictMode lifecycle.

Generated-by: Codex
@liuxiaocs7
chore: merge latest main into Skill locations PR
804868b 
Resolve the compatible protocol declaration at main's epoch 165 and retain the reviewed Skill location fixes.

Generated-by: Codex
@liuxiaocs7
liuxiaocs7 merged commit fcc8db8 into apache:main Sep 19, 2026
2 of 4 checks passed
@liuxiaocs7

liuxiaocs7 commented Sep 19, 2026

Copy link
Copy Markdown
Member Author

Thanks @Astro-Han for reviewing!

@Joob1n Joob1n mentioned this pull request Sep 19, 2026
Merged
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Astro-Han Astro-Han Astro-Han approved these changes

Assignees

No one assigned

Labels

effort/XL Under 2500 readable lines

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

bug(desktop): Open Skills folder ignores standard discovery locations

2 participants

@liuxiaocs7 @Astro-Han